Skip to content

Implement get_serialize_properties() handler #2419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Mar 13, 2017

Unfortunately it looks like we can't use this in ext/date, as ext/date also supports __set_state and relies on get_properties() for that. I'm wondering whether we oughtn't be respecting __sleep for var_export as well, in which case it could use get_serialize_properties() too.

nikic added 3 commits March 13, 2017 17:38
Decouple handling of __sleep() from remaining serialization logic
completely.

In particular, create an explicit HT with the properties that are
to be serialized. To compensate, remove the previously existing names
HT and perform duplicate property handling on the fly.

A side benefit of doing it this way is that duplicate properties will
be detected even if they only clash after name mangling.
The signature is intentionally the same as for get_debug_info(),
as these can commonly share an implementation.
@bukka
Copy link
Member

bukka commented Apr 1, 2017

I remember asking about something similar few years ago and there was some opposition so might be good idea to discuss it again maybe...

https://www.mail-archive.com/[email protected]/msg66293.html

@krakjoe
Copy link
Member

krakjoe commented Jul 25, 2017

@nikic can we get a status update here please ?

@nikic
Copy link
Member Author

nikic commented Oct 23, 2018

This has been superseded by #3579. The first commit here, which cleans up __sleep handling, might still be applicable, but it seems to have minor behavior changes, so it's safer to just leave it alone.

@nikic nikic closed this Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants